atc-installer: configure validation webhook timeout#211
Open
alam-cloud wants to merge 3 commits intoyokecd:mainfrom
Open
atc-installer: configure validation webhook timeout#211alam-cloud wants to merge 3 commits intoyokecd:mainfrom
alam-cloud wants to merge 3 commits intoyokecd:mainfrom
Conversation
Add support for configuring validation webhook timeout when installing the ATC. - Add ValidationWebhookTimeout field to installer Config - Pass timeout as VALIDATION_WEBHOOK_TIMEOUT environment variable - Load timeout from env var in ATC config - Apply timeout to all validation webhooks (default: 10s) Fixes yokecd#129
Author
|
Key changes:
The timeout is optional - if not specified, it defaults to 10 seconds (Kubernetes default). Let me know if you'd like any changes! 👍 |
davidmdm
reviewed
Nov 22, 2025
cmd/atc-installer/installer/run.go
Outdated
| DockerConfigSecretName string `json:"dockerConfigSecretName,omitzero" Description:"name of dockerconfig secret to allow atc to pull images from private registries"` | ||
| LogFormat string `json:"logFormat,omitzero" Enum:"json,text"` | ||
| Verbose bool `json:"verbose,omitzero" Description:"verbose logging"` | ||
| ValidationWebhookTimeout *int32 `json:"validationWebhookTimeout,omitzero" Description:"timeout in seconds for validation webhooks (default: 10)"` |
Collaborator
There was a problem hiding this comment.
I think a pointer is not necessary in this case. If the value is 0 or less than 0 we can ignore the field.
davidmdm
reviewed
Nov 22, 2025
cmd/atc-installer/installer/run.go
Outdated
| if cfg.ValidationWebhookTimeout != nil { | ||
| env = append(env, corev1.EnvVar{ | ||
| Name: "VALIDATION_WEBHOOK_TIMEOUT", | ||
| Value: strconv.FormatInt(int64(*cfg.ValidationWebhookTimeout), 10), |
Collaborator
There was a problem hiding this comment.
if we use an int which is completely reasonable as the installer does not need to be optimized (its run once or once in a while) we could use the strconv.Itoa and avoid the type casts and dereferencing.
davidmdm
reviewed
Nov 22, 2025
cmd/atc/config.go
Outdated
| } | ||
|
|
||
| cfg.Service.CABundle = cfg.TLS.CA.Data | ||
| cfg.Service.ValidationWebhookTimeout = cfg.ValidationWebhookTimeout |
Collaborator
There was a problem hiding this comment.
I dont' think we need to put the timeout on the service structure. That is intended to define the k8s service that backs the atc and its tls requirements.
davidmdm
reviewed
Nov 22, 2025
cmd/atc/resources.go
Outdated
| } | ||
|
|
||
| // Get timeout value, defaulting to 10 seconds if not configured | ||
| timeoutSeconds := cmp.Or(cfg.ValidationWebhookTimeout, ptr.To[int32](10)) |
Collaborator
There was a problem hiding this comment.
It might be a good idea to have different timeout values? Instead of one global flag for all webhooks have three different ones?
davidmdm
reviewed
Nov 22, 2025
Collaborator
davidmdm
left a comment
davidmdm
left a comment
There was a problem hiding this comment.
Awesome start! Love to see it!
added 2 commits
November 23, 2025 09:27
…viceDef - Change ValidationWebhookTimeout from *int32 to int32 in installer Config - Use strconv.Itoa instead of pointer dereferencing - Remove ValidationWebhookTimeout from ServiceDef (it's for k8s service definition) - Pass timeout as separate parameter to GetReconciler - Add timeout field to atc struct instead Addresses review comments from @davidmdm
- Replace single ValidationWebhookTimeout with three separate fields: - AirwayValidationWebhookTimeout (default: 10s) for airway instances - ResourceValidationWebhookTimeout (default: 5s) for event dispatching - ExternalResourceValidationWebhookTimeout (default: 30s) for external resources - Update installer Config and ATC Config to support separate timeouts - Apply appropriate timeout to each webhook type based on its purpose - Update GetReconciler to use AirwayValidationWebhookTimeout for per-Airway webhooks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add support for configuring validation webhook timeout when installing the ATC.
Fixes #129